Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete nested tag inside codeFence #564

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Delete nested tag inside codeFence #564

merged 5 commits into from
Aug 14, 2023

Conversation

akamefi202
Copy link
Contributor

Added a new rule to htmlToMarkdownRules to remove unnecessary nested tags inside <pre> tag.

Fixed Issues

Expensify/App#23659

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    In ExpensiMark-Markdown-test.js file, test named Test codeFence copy from selection does not add extra new line.
  2. What tests did you perform that validates your changed worked?
    Checked if htmlToMarkdown removes unnecessary tags correctly so extra newline character isn't added.

QA

  1. What does QA need to do to validate your changes?
    Run the new test cases.
  2. What areas to they need to test for regressions?
    N/A

@akamefi202 akamefi202 requested a review from a team as a code owner August 9, 2023 07:07
@github-actions
Copy link

github-actions bot commented Aug 9, 2023 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from jasperhuangg and removed request for a team August 9, 2023 07:08
@akamefi202
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@akamefi202 akamefi202 mentioned this pull request Aug 9, 2023
Closed
6 tasks
@jasperhuangg
Copy link
Contributor

@puneetlath assigning you since this is related to an issue you're managing

@jasperhuangg jasperhuangg removed their request for review August 9, 2023 17:16
@puneetlath
Copy link
Contributor

@mollfpr please take the first review.

@mollfpr
Copy link
Contributor

mollfpr commented Aug 11, 2023

Sorry for the delay guys. I got cold yesterday 🙏

@akamefi202 You need to sign the commit https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request

For the CLA, I guess you can comment recheck for the action to rerun.

@akamefi202
delete nested tag inside pre tag
2f15a7c 
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202
added expensify markdown test
c184a61 
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202
update commit
f06ecfd 
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202
update commit1
5de42e4 
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202
gpg sign
23a1a44 
Signed-off-by: akamefi202 <akamefi202@gmail.com>
mollfpr
mollfpr reviewed Aug 14, 2023
View reviewed changes
package-lock.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akamefi202 Could you revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did.

mollfpr
mollfpr approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested great on the ND and seems the tests are run pretty well 👍

puneetlath
puneetlath reviewed Aug 14, 2023
View reviewed changes
__tests__/ExpensiMark-Markdown-test.js
testString = '<pre>code<br></pre>text';
expect(parser.htmlToMarkdown(testString)).toBe('```\ncode\n```\ntext');

testString = '<h3>test heading</h3><div><pre class=\"notranslate\"><code class=\"notranslate\">Code snippet\n</code></pre></div><blockquote><p><a href=\"https://www.example.com\">link</a></p></blockquote>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why are we using an h3 in this test instead of an h1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we copy & paste selection from Github, heading is shown as h3 tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah cool, I see.

@puneetlath
Copy link
Contributor

Looks good to me! @akamefi202 you'll next need to open a PR in the App repo to point to the updated expensify-common commit hash.

@puneetlath puneetlath merged commit 4cc5f72 into Expensify:main Aug 14, 2023
3 checks passed
@akamefi202 akamefi202 deleted the selection-copy-markdown branch August 14, 2023 17:46
@akamefi202
Copy link
Contributor Author

@puneetlath @mollfpr Thanks. I will create a PR in the App repo too!

@akamefi202 akamefi202 mentioned this pull request Aug 14, 2023
Merged
59 tasks
@akamefi202
Copy link
Contributor Author

@puneetlath @mollfpr I created the PR in App repo.
Expensify/App#24549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puneetlath puneetlath puneetlath approved these changes

@mollfpr mollfpr mollfpr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akamefi202 @jasperhuangg @puneetlath @mollfpr